[SPARK-60103][SQL] Length-check XML CHAR/VARCHAR map keys without pad or trim - #59316

Review summary

The flag-on key checks are implemented consistently across element names, prefixed attributes, and the valueTag, and the tests cover PERMISSIVE/FAILFAST, ignoreCorruptFiles, nested maps, ARRAY<MAP>, and sibling recovery well. The main issue is outside the declared scope: the new direct MapType arm in convertObject changes results for ordinary MAP<STRING, _> struct fields with the flag off (empty, attribute-only, and text-only elements), and nothing documents or tests that. The widened key-type match also lets non-string key types reach convertMap through the unvalidated format("xml").load() path, where they now fail with a ClassCastException instead of a malformed record. Two text issues remain: the convertMap scaladoc omits the flag condition, and the migration-guide entry misstates the flag-off precondition and the 4.3 baseline. Collated keys keeping both ab and AB under exact-name last-win is consistent with the PR's stated design and is not raised.

Review-level concerns

• Shared problem: The XML map dispatch rewrite reaches beyond the declared CHAR/VARCHAR key change. The new convertObject MapType arm changes MAP<STRING, _> results for empty, attribute-only, and text-only elements with the flag off. The catch-all MapType(kt, ...) lets non-string key types produce mistyped maps that fail with ClassCastException. Narrow the dispatch in StaxXmlParser together: drop the direct struct-field arm and restrict the remaining map arm to StringType-family keys. That keeps the new key-length checks and returns everything else to merge-target behavior. Both defects are P2/P3, so this is non-blocking.

Findings

4 total: 0 P0, 0 P1, 1 P2, 3 P3.

Non-blocking (P2)

• New struct-field MapType arm changes ordinary MAP<STRING, _> results with the flag off — General
  The new case mt: MapType => convertMap(...) arm in convertObject (StaxXmlParser.scala ~650) routes every map-typed struct field around convertField's peek-based dispatch. That includes plain MAP<STRING, _> fields with spark.sql.charVarchar.standardSemantics.enabled off. Under default configuration, for schema("m MAP<STRING, STRING>"):

  • <ROW><m/></ROW> used to give m = NULL (convertField's (EndElement, _) arm) and now gives an empty map.
  • <ROW><m a="1"/></ROW> used to give NULL and now gives {_a -> 1}.
  • <ROW><m>xy</m></ROW> used to be a malformed record (convertTo raises _LEGACY_ERROR_TEMP_3246) and now gives {_VALUE -> xy}.

  The same element inside ARRAY<MAP<...>> or as a nested map value still goes through convertField, so the result now depends on where the map sits. The PR description and migration note scope the change to CHAR/VARCHAR keys under the flag, and no test pins these STRING-key results. Unless this change is intended and documented, struct-field maps should keep going through convertField. Its StartElement path already reaches convertMap, and with it the new key checks, for non-empty elements.

  Recommended change: Remove the direct MapType arm from convertObject so struct-field maps use convertField's existing dispatch again. Update the CharVarcharTestSuite cases that depended on that arm: attribute-only <m ab="1"></m> and text-only <m>xy</m> should assert the restored results or use element-content inputs that still exercise the prefixed-attribute and valueTag key checks. Add XML map tests pinning the merge-target results for empty, attribute-only, and text-only MAP<STRING, _> struct fields.

  Why this works: convertField's peek decides empty and text-only elements before convertMap runs, so MAP<STRING, _> results return to merge-target behavior. For non-empty elements, convertComplicatedType still calls convertMap with the element's attributes, so the CHAR/VARCHAR key checks and bounded-drain recovery are unchanged.

  Scope: Restore the merge-target struct-field map dispatch in the XML parser and realign the affected CHAR/VARCHAR tests, adding STRING-key regression coverage.

  Compatibility: UNSUPPORTED_XML_CHAR_VARCHAR_MAP_KEY checks and sibling recovery for non-empty CHAR/VARCHAR map elements.

  Risks: Tests that pinned the attribute-only and text-only results of the direct arm must be updated, not deleted, so the prefixed-attribute and valueTag key checks stay covered.

  Constraints: Keep the bounded-drain sibling recovery for key and value failures in non-empty map elements.

  Success: With default configuration, a MAP<STRING, _> struct field whose element is empty or whitespace-only is null, an attribute-only element is null, and a text-only element is a malformed record, as in the merge target. The same map element gives the same result as a struct field, an ARRAY element, and a nested map value. CHAR/VARCHAR key length checks under the flag still apply to every non-empty map element, including prefixed attributes and the valueTag in mixed content, with sibling recovery.

  Verification:

  • Behavior: from_xml and XML datasource reads of MAP<STRING, _> fields with empty, attribute-only, and text-only elements return the merge-target null or malformed-record results under default configuration.
  • Compatibility: Struct-field, ARRAY element, and nested-map-value placements of the same map element produce the same result.
  • Regression: CHAR/VARCHAR key-check coverage (PERMISSIVE/FAILFAST, nested maps, ARRAY, tail recovery, prefixed attributes, valueTag in mixed content) still passes.

Nit (P3)

• Widened map dispatch lets non-string key types build mistyped maps — General
convertComplicatedType's map arm is now case MapType(kt, vt, _) (StaxXmlParser.scala ~388), and the new convertObject arm accepts any MapType. For a non-CHAR/VARCHAR key type, convertXmlMapKey falls back to CharVarcharUtils.applyTextParseSemantics, which returns the UTF8String unchanged. ExprUtils.checkXmlSchema (INVALID_XML_MAP_KEY_TYPE) runs only for from_xml and DataFrameReader/DataStreamReader.xml(). Meanwhile spark.read.format("xml").schema("m MAP<INT, STRING>").load(path) and CREATE TABLE ... USING xml skip it, and XmlFileFormat.supportDataType accepts atomic key types. On those paths, <ROW><m><a>x</a></m></ROW> used to hit a MatchError that became a malformed record (NULL in PERMISSIVE). It now builds MapData with UTF8String keys under IntegerType, which fails in the scan projection (getInt) with a ClassCastException in every parse mode. Restricting the map arm to string-family key types (for example MapType(_: StringType, vt, _), which CHAR/VARCHAR and collated strings satisfy) would keep the old failure mode.

  Verification:

  • Regression: Reading m MAP<INT, STRING> through format("xml").load() returns null in PERMISSIVE and fails as a malformed record in FAILFAST, with no ClassCastException.
  • Behavior: Existing CHAR/VARCHAR and collated XML map-key tests still pass.

• convertMap scaladoc states no-pad key semantics without the flag condition — General
  The convertMap scaladoc ("XML names used as CHAR/VARCHAR keys are length-checked without rewriting ... Padding, trimming, and mapKeyDedupPolicy are not applied", plus the MAP<CHAR(4), INT> example) reads as unconditional. convertXmlMapKey applies those semantics only when charVarcharStandardSemantics is true. Otherwise keys go through CharVarcharUtils.applyTextParseSemantics, which pads CHAR keys and raises EXCEED_LIMIT_LENGTH, as the flag-off test's expected key 'a ' shows. Qualifying the paragraph with the flag condition would keep it accurate.

  Verification:

  • Inspection: The scaladoc's stated key semantics match convertXmlMapKey for both flag values and agree with the flag-off test's padded-key expectation.

• Migration-guide entry misstates the flag-off precondition, the 4.3 baseline, and the checked keys — General
  A few things in the new docs/sql-migration-guide.md bullet could mislead upgrading users:

  • "With the flag off, CHAR keys are still padded and VARCHAR overflow still uses EXCEED_LIMIT_LENGTH" holds only when spark.sql.preserveCharVarcharTypeInfo is true. The flag-off test sets it, and the adjacent bullet states that precondition. With both flags at their defaults, CHAR/VARCHAR schema types never reach the parser.
  • "still" doesn't match 4.3. In branch-4.3 the only XML map arm is case MapType(StringType, vt, _), and StringType.equals compares the length constraint, so MAP<CHAR(n)/VARCHAR(n), _> XML maps were malformed records there. Padding is new in 4.4.
  • The checked keys include the attributePrefix + local name (default _ab) and the valueTag (default _VALUE). Under the flag, any CHAR(n != 6) / VARCHAR(n < 6) map whose element has text content therefore fails, which users would want to know.
  • "XML CHAR/VARCHAR values still use write-side pad" describes a parse-time check with write-path wording, while the bullet just above calls the same check "Read-side length checks".

  Verification:

  • Inspection: Each statement in the bullet matches convertXmlMapKey/convertMap behavior, the flag-off test's preconditions, and upstream/branch-4.3's map dispatch.

PR description suggestions

• The body's "With the flag off, CHAR keys are still padded and VARCHAR overflow still uses EXCEED_LIMIT_LENGTH" applies only when spark.sql.preserveCharVarcharTypeInfo is true. Consider stating that precondition, as the adjacent 4.4 migration bullet does.

Generated by Omnigent on Databricks.

Review summary

This round reviews 8e1c55b. The previous review at 02b478e raised four findings. Three are fully addressed. Struct-field maps go through convertField again, so MAP<STRING, _> results match the merge target. The map arm admits only string-family keys, so MAP<INT, _> read through format("xml").load() is a malformed record again, with a test pinning it. The convertMap scaladoc now states the flag condition. The fourth, migration-guide accuracy, is mostly addressed: the bullet now names the checked keys and the preserveCharVarcharTypeInfo precondition, and its wording is fixed.

Two non-blocking P3 nits remain. First, the 4.3 sentence added to the migration bullet doesn't match branch-4.3 for from_xml and reader schemas, and the bullet and the scaladoc omit that flag-off over-length CHAR keys raise EXCEED_LIMIT_LENGTH. That sentence follows the earlier review's own description of 4.3, which looked only at parser dispatch and was incomplete. Second, the DUPLICATED_MAP_KEY routing in StaxXmlParser, the DuplicateMapKeyUtils object, and two test helpers no longer have a producer. That was already true at 02b478e, and the earlier review missed it.

There is also one question about whether the new key checks should follow the flag's persisted view value rather than the caller's session.

Findings

2 total: 0 P0, 0 P1, 0 P2, 2 P3.

Nit (P3)

• Migration note misstates the 4.3 baseline and omits flag-off CHAR key overflow — docs/sql-migration-guide.md:32 — see inline.

• Dead DUPLICATED_MAP_KEY routing and test helpers left after removing XML dedup — General
  With convertConstrainedMap and DuplicateMapKeyUtils.buildConstrainedMap removed, nothing on the XML parse path can raise DUPLICATED_MAP_KEY anymore; convertMap builds ArrayBasedMapData(kvPairs.toMap). The SPARK-59274 plumbing that let that error escape parse-mode handling is still there:

  • DuplicateMapKeyUtils, now only cause/unapply, and its import in StaxXmlParser;
  • five special cases in StaxXmlParser: the case DuplicateMapKeyUtils(e) => throw e arms in doParseColumn (line 238), assignAttributes (560), and convertObject (699); the DuplicateMapKeyUtils.cause branch in doParseColumnOptimized's PartialResultException arm (335); and the root-cause arm in its Throwable handler (344);
  • the private assertDuplicateMapKey / assertDuplicateMapKeyError helpers in CharVarcharTestSuite (line 1069), which no test calls anymore.

  None of this can run, but it tells readers that XML parsing can still raise DUPLICATED_MAP_KEY and that the error must bypass PERMISSIVE and ignoreCorruptFiles. That contradicts the updated spark.sql.mapKeyDedupPolicy doc. Any future producer of that error would inherit the bypass without anyone having decided it should. Deleting the object, the five arms, and the two helpers changes no behavior; after the deletion, the PartialResultException arm simply always throws BadRecordException.

  Verification:

  • Compile: The catalyst main sources and the sql core test sources compile with DuplicateMapKeyUtils and the helpers removed.
  • Regression: Existing XML CHAR/VARCHAR map-key and value tests keep passing under PERMISSIVE, FAILFAST, and ignoreCorruptFiles, including sibling recovery after a key-check failure.
  • Inspection: No remaining reference to DuplicateMapKeyUtils or DUPLICATED_MAP_KEY special-casing exists in the XML parser.

Re-review status

Prior AI findings: 4 addressed, 0 still present; additional unresolved findings in this review: 2.

New attribution: 1 newly introduced, 1 late catch, 0 previously raised, 0 unattributed.

Remaining prior AI findings

No prior AI findings remain.

Existing discussions

• existing discussion — Each claim checked against 8e1c55b. (1) convertObject sends map-typed struct fields through convertField again, so empty and attribute-only elements hit the EndElement arm (NULL) and text-only elements hit convertTo (malformed); new tests pin all three plus an ARRAY empty element. (2) convertComplicatedType matches only MapType(kt: StringType, ...), and a new format("xml").load() test asserts MAP<INT, STRING> is a malformed record with no ClassCastException. (3) The prefixed-attribute and valueTag tests use mixed content. (4) The convertMap scaladoc qualifies the no-pad semantics with the flag, and the migration bullet names attributePrefix/valueTag and the preserveCharVarcharTypeInfo precondition. The '4.3 malformed-record baseline' the author added, at the earlier review's prompting, is inaccurate. In branch-4.3, from_xml and DataFrameReader schemas rejected CHAR/VARCHAR at analysis (UNSUPPORTED_CHAR_OR_VARCHAR_AS_STRING) or mapped them to STRING under spark.sql.legacy.charVarcharAsString. The earlier review described only the parser dispatch. That correction, plus the omitted flag-off CHAR overflow error, is the related migration-guide finding.

PR description suggestions

• The body repeats the migration bullet's "In Spark 4.3, MAP<CHAR(n), _> / MAP<VARCHAR(n), _> XML maps were malformed records" and describes flag-off CHAR keys only as padded. Consider updating both to match the corrected note. In 4.3, from_xml and reader schemas rejected CHAR/VARCHAR, or treated them as STRING under spark.sql.legacy.charVarcharAsString. With the flag off, over-length CHAR keys also raise EXCEED_LIMIT_LENGTH.

Decision challenges

Should XML CHAR/VARCHAR key checks follow the view's persisted flag value?

StaxXmlParser now reads spark.sql.charVarchar.standardSemantics.enabled from SQLConf.get when the parser is constructed (line 89). For from_xml that happens when the evaluator is built at execution; the XML file readers build a parser per task.

What I verified: the flag is declared with ConfigBindingPolicy.PERSISTED, and its comment says a view created under standard semantics must keep CHAR/VARCHAR behavior regardless of the caller's session. effectiveSQLConf is consulted only during view resolution (ViewResolution, ViewResolver, SessionCatalog), and neither XmlToStructs nor XmlOptions captures the flag. In the merge target, CHAR/VARCHAR XML map keys always went through applyTextParseSemantics, so key handling did not depend on the flag. Take a view created with the flag on over from_xml(x, 'm MAP<CHAR(2), INT>'). It keeps CHAR(2) in its schema, but a caller with the flag off appears to get the flag-off key path: a is padded to a instead of rejected, and over-length keys raise EXCEED_LIMIT_LENGTH instead of UNSUPPORTED_XML_CHAR_VARCHAR_MAP_KEY.

What I couldn't confirm: I didn't run a cross-session view query. ToStringBase also reads this flag at runtime for CAST, so runtime binding may be intended for the feature. Is it intended here? If not, the flag could be captured at analysis, for example on XmlToStructs and the XML read options the way timeZoneId is, and passed to the parser.

Generated by Omnigent on Databricks.